-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CNI: add fallback logic if no ip address references sandboxed interface #9895
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to unit test this? Maybe split the cni.Setup
code and the address-finding-code into separate funcs as the latter seems unittestable. I'm actually less worried about correctness than I am about having a test as a sort of documentation for intended behavior. The "use first IP address we find" seems like strange behavior to me as someone who is not familiar with CNI plugin behavior.
The goal is to always find an interface with an address, preferring sandbox interfaces, but falling back to the first address found. A test was added against a known CNI plugin output that was not handled correctly before.
I refactored the CNIResult -> AllocNetworkStatus conversion into a new func for unit testing. I'd love to test any more configurations if you have them handy! Sadly its still possible to get nondeterministic results from this new func, but I think the interface precedence logic is correct. I think I may error in more cases than before. Before we would only error if Now we also error if there are no interfaces with an IP address set. I think that was the intention of the original code, but not the actual behavior. |
Thanks for digging into this @schmichael I approve of these changes (even though I can't approve my own PR). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving my own changes since @nickethier can't approve his own PR. 😅
docs: add #9895 to the changelog
CNI: add fallback logic if no ip address references sandboxed interface
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR adds a branch of logic to how the alloc's IP address is discovered for CNI networks. Some CNI plugins don't reference an interface index in the result response. This leads to go-cni, the library we use for CNI, to associate the address with the default interface.
This logic will attempt to cycle through the interfaces and find the first address if there is no address found for the sandboxed interface.
Some additional debug log statements are also included to aid future CNI debugging.